Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not apply a refactoring if it drops comments #101

Merged
merged 1 commit into from
Nov 22, 2020

Conversation

zliu41
Copy link
Collaborator

@zliu41 zliu41 commented Nov 22, 2020

As a result, we no longer filter overlapping hints out before applying them; instead, we filter them out as we go. Because if refactoring A overlaps with refactoring B, but A isn't applied due to dropping comments, then B can still be applied.

Fixes #93

@jneira
Copy link
Contributor

jneira commented Jan 13, 2021

Hi @zliu41 , not sure if this is related with a broken test case in hls using apply-refact head:

apply-refact preserve regular comments:                                                           FAIL (1.64s)
  test/functional/FunctionalCodeAction.hs:188:
  expected: "-- comment before header\nmodule ApplyRefact6 where\n\n{-# standalone annotation #-}\n\n-- standalone comment\n\n-- | haddock comment\nf = {- inline comment -} 1 -- ending comment\n\n-- final comment\n"
  but got: "-- comment before header\nmodule ApplyRefact6 where\n\n{-# standalone annotation #-}\n\n-- standalone comment\n\n-- | haddock comment\nf = {- inline comment -} (1) -- ending comment\n\n-- final comment\n"

The hint is not applied but in previous runs using 4fbd3a3 with the same hls code, the test was succesful (so it applied the hint and preserved comments)

The code for ghc-8.6.5 uses the old applyRefactoring

I am gonna use 4fbd3a3 again to see if the test case is green again and will confirm here the result

@jneira
Copy link
Contributor

jneira commented Jan 13, 2021

Well i did not test a inline comment inside the code being refactored:

({- inline comment inside refactored code -}1)

So maybe the refactoring would remove it (i will add that case in my test suite). If that is the case, maybe the code could detect if the comment is inside the source span being refactored and no outside, to no apply the hint

@jneira
Copy link
Contributor

jneira commented Jan 13, 2021

Mmm reviewing the test cases here i think {- inline comment -} is being attached to ( and, as the refactor will drop (, it preserves the comment, not applying the hint.

Not sure if that is which i would expect, i guess the code is being conservative, as the comment could refer the tokens being removed in some way, or is there another reason?

@jneira
Copy link
Contributor

jneira commented Jan 13, 2021

After use 4fbd3a3 with haskell/haskell-language-server@7144430 the test is succesful again: https://github.com/haskell/haskell-language-server/pull/635/checks?check_run_id=1693884153

I'am gonna add a comment inside refactored code in the test case for completeness

@jneira
Copy link
Contributor

jneira commented Jan 13, 2021

The test case has failed with

         test/functional/FunctionalCodeAction.hs:188:
        expected: "-- comment before header\nmodule ApplyRefact6 where\n\n{-# standalone annotation #-}\n\n-- standalone comment\n\n-- | haddock comment\nf = {- inline comment -} {- inline comment inside refactored code -}1 -- ending comment\n\n-- final comment\n"
         but got: "-- comment before header\nmodule ApplyRefact6 where\n\n{-# standalone annotation #-}\n\n-- standalone comment\n\n-- | haddock comment\nf = {- inline comment -}{- inline comment inside refactored code -} 1 -- ending comment\n\n-- final comment\n"

So 4fbd3a3 preserve comments inside the refactored code but it removed one existent whitespace before

The file being tested is

-- comment before header
module ApplyRefact6 where

{-# standalone annotation #-}

-- standalone comment

-- | haddock comment
f = {- inline comment -} ({- inline comment inside refactored code -}1) -- ending comment

-- final comment

@zliu41
Copy link
Collaborator Author

zliu41 commented Jan 14, 2021

@jneira Thanks for reporting these problems! In the {- comment -} (1) case, the refactoring is supposed to be performed since it wouldn't drop the comment. So this is a bug. What happened is that when a comment is attached to the entire hint, it is always preserved. In this case, the comment is attached to the HsPar, and the HsPar is the entire hint. And so the comment is preserved. This is a behavior I wasn't aware of. The fix should be easy; I'll try to fix it tomorrow.

I'll take a closer look at the ({- comment -} 1) case and see how to make the whitespaces right.

@zliu41
Copy link
Collaborator Author

zliu41 commented Jan 15, 2021

@jneira I have fixed the {- comment -} (1) case. Will make a release tomorrow.

The ({- comment -}1) case is trickier. Here the comment has a DeltaPos of (0, 0), meaning it appears immediately after the previous object, (. So when the ( is removed, we'd need to update its DeltaPos appropriately, but I haven't found a clean and uniform way to do that. This won't be fixed before the release, and I opened #105 for tracking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apply hint: use guards remove comments above
2 participants